Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor #165

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Refactor #165

wants to merge 11 commits into from

Conversation

LucasPaganini
Copy link

Hey, I was reading the source code to better understand how the smooth scroll works and how one would polyfill it. I found myself making some comments to better understand the code and I thought it could be a small contribution for future developers also trying to understand it.

This PR:

  • Updates prettier to its latest version
  • Adds more JSDoc types

All changes are cosmetic and I totally understand if you decide not to merge this PR since it's not addressing any existing issues.

Thanks for the good code, I've learned a lot!

`'a' instanceof String => false` and `true instanceof Boolean => false`, the capitalized versions of JavaScript primitive types are avoided because they lead us to think that we're dealing with actual instances of those classes, when we're most often not (as in our case).
In TypeScript, we avoid the usage of `Object`, favoring `object` instead. Also, we use `void` as the return type of functions that return `undefined`, this indicates that we don't really care that it outputs `undefined`, it is `void`, which means that it's nothing and the return of it should be ignored.
By casting `'overflowX'|'overflowY'` to the `overflowProperty`, we tell the compiler (or type checker) which property we'll be accesing from the `CSSStyleDeclaration`. That's useful for IDEs such as VSCode, that uses the TypeScript parser to analyze the code. It can also be useful if we decide to add a type checker to the codebase later on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant